Add strict mode for DataLoaderRegistry#252
Open
AlexandreCarlton wants to merge 2 commits intographql-java:masterfrom
Open
Add strict mode for DataLoaderRegistry#252AlexandreCarlton wants to merge 2 commits intographql-java:masterfrom
AlexandreCarlton wants to merge 2 commits intographql-java:masterfrom
Conversation
In a similar vein to graphql-java/graphql-java#3565 which enabled "strict mode" for type wiring (preventing multiple `DataFetcher`s being registered to the same field on a GraphQL type), we add "strict mode" to the `DataLoaderRegistry` so that we don't accidentally register multiple DataLoaders to the same key (which would result in the last registration winning). This defaults to `false` to avoid breaking changes.
bbakerman
requested changes
Nov 24, 2025
Member
bbakerman
left a comment
bbakerman
left a comment
There was a problem hiding this comment.
Move the strict check in nameAndInstrumentDL as a common place and all the registering methods will then be covered
| if (strictMode) { | ||
| assertKeyStrictly(name); | ||
| } | ||
| dataLoaders.put(name, nameAndInstrumentDL(name, instrumentation, dataLoader)); |
Member
There was a problem hiding this comment.
You need to cover org.dataloader.DataLoaderRegistry#computeIfAbsent as well
I think this code would be better put in the nameAndInstrumentDL method as a common place
The following is all the places it is called
instrumentDLs(Map<String, DataLoader<?, ?>>, DataLoaderInstrumentation)
register(DataLoader<?, ?>)
register(String, DataLoader<?, ?>)
registerAndGet(String, DataLoader<?, ?>)
computeIfAbsent(String, Function<String, DataLoader<?, ?>>)
Collaborator
Author
There was a problem hiding this comment.
Done - as agreed elsewhere, I've omitted computeIfAbsent since this repeated invocations to the same key will not override the DataLoader with strict mode off.
I did have to make nameAndInstrumentDL to get access to strictMode and the DataLoader map, however.
This is done to ensure coverage of all registering locations. We purposefully omit `computeIfAbsent` as this does _not_ register a new DataLoader if there is an existing key.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
In a similar vein to graphql-java/graphql-java#3565 which enabled "strict mode" for type wiring (preventing multiple
DataFetchers being registered to the same field on a GraphQL type), we add "strict mode" to theDataLoaderRegistryso that we don't accidentally register multiple DataLoaders to the same key (which would result in the last registration winning). This can prevent confusing bugs from emerging.This defaults to
falseto avoid breaking changes, and deliberately mimics the referenced PR to maintain consistency.